Conversation
typescript-bot
added
the
For Uncommitted Bug
There was a problem hiding this comment.
Pull request overview
Adds support for the FORCE_COLOR environment variable to control tsc’s “pretty” (colored) output, while ensuring NO_COLOR continues to disable color even when FORCE_COLOR is present.
Changes:
- Update pretty-output defaulting logic to honor
FORCE_COLOR(and keepNO_COLORas an override). - Add command-line unit tests for
FORCE_COLORandNO_COLOR+FORCE_COLORprecedence. - Add new reference baselines for the new test scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/compiler/executeCommandLine.ts | Updates defaultIsPretty to consider NO_COLOR and FORCE_COLOR. |
| src/testRunner/unittests/tsc/commandLine.ts | Adds verifyTsc cases covering FORCE_COLOR and precedence with NO_COLOR. |
| tests/baselines/reference/tsc/commandLine/adds-color-when-FORCE_COLOR-is-set.js | Baseline output for the new FORCE_COLOR scenario. |
| tests/baselines/reference/tsc/commandLine/does-not-add-color-when-NO_COLOR-is-set-even-if-FORCE_COLOR-is-set.js | Baseline output ensuring NO_COLOR disables color even when FORCE_COLOR is set. |
| verifyTsc({ | ||
| scenario: "commandLine", | ||
| subScenario: "adds color when FORCE_COLOR is set", | ||
| sys: () => | ||
| TestServerHost.createWatchedSystem(emptyArray, { | ||
| environmentVariables: new Map([["FORCE_COLOR", "true"]]), | ||
| }), | ||
| commandLineArgs: emptyArray, | ||
| }); |
There was a problem hiding this comment.
The new "adds color when FORCE_COLOR is set" test uses TestServerHost.createWatchedSystem, whose writeOutputIsTTY() always returns true. That means the output would already be colored even without FORCE_COLOR, so this test doesn't actually validate the new FORCE_COLOR behavior (forcing color in a non-TTY scenario). Consider overriding writeOutputIsTTY to return false for this test (or enhancing the host to support a non-TTY mode) so the baseline would differ without FORCE_COLOR and the test would catch regressions.
| if (sys.getEnvironmentVariable("NO_COLOR")) { | ||
| return false; | ||
| } | ||
| if (sys.getEnvironmentVariable("FORCE_COLOR")) { |
There was a problem hiding this comment.
FORCE_COLOR is treated as enabled for any non-empty value, but in common FORCE_COLOR semantics a value of "0" explicitly disables color. With the current check, FORCE_COLOR=0 will incorrectly force pretty output on. Consider parsing the value and treating "0" (and possibly "false") as disabling, while still allowing "1"/"2"/"3" to force-enable.
| if (sys.getEnvironmentVariable("FORCE_COLOR")) { | |
| const forceColor = sys.getEnvironmentVariable("FORCE_COLOR"); | |
| if (forceColor !== undefined) { | |
| const normalized = forceColor.trim().toLowerCase(); | |
| if (normalized === "0" || normalized === "false") { | |
| return false; | |
| } |
Same as microsoft/typescript-go#2596